feat: Impl InMemoryCatalog's UpdateTable #386
Conversation
WZhuo
force-pushed
the
memory_catalog
branch
4 times, most recently
from
55d3900 to
67a5362
Compare
| const std::vector<std::unique_ptr<TableRequirement>>& requirements, | ||
| const std::vector<std::unique_ptr<TableUpdate>>& updates) { | ||
| return NotImplemented("update table"); | ||
| std::unique_lock lock(mutex_); |
There was a problem hiding this comment.
| std::unique_lock lock(mutex_); | |
| std::lock_guard guard(mutex_); |
Use std::lock_guard instead of std::unique_lock if we don't need to move the guard.
There was a problem hiding this comment.
I think it can be optimized with ReadWriteLock later.
There was a problem hiding this comment.
I agree that we may be able to replace the lock type with std::shared_lock. If you don't do that in this PR, I hope that you can replace with std::lock_guard first. Because we have merged a PR #398 which replaced the other unique_lock in this file.
| return TableMetadataFromJson(json); | ||
| } | ||
|
|
||
| Status TableMetadataUtil::Write(FileIO& io, const TableMetadata* base, |
There was a problem hiding this comment.
How about using reference instead of raw pointer?
There was a problem hiding this comment.
The base can be nullptr, indicating that this is creating a new table.
There was a problem hiding this comment.
You can turn metadata into reference.
| /// \return The file extension of the codec. | ||
| static std::string CodecTypeToFileExtension(MetadataFileCodecType codec); | ||
|
|
||
| inline static constexpr std::string_view kTableMetadataFileSuffix = ".metadata.json"; |
There was a problem hiding this comment.
static constexpr is implicitly inline and we can remove inline here.
There was a problem hiding this comment.
It's implicitly inline in c++20+, It's recommended to explicitly define as inline in my previous PR.
There was a problem hiding this comment.
Sorry that I have no time last week so I didn't review your previous PR. This project is under at least C++23, so we don't need inline to avoid ODR violations.
3 participants
No description provided.